Skip to content

fix(gateway): honor per-platform session isolation overrides - #84925

Open
Diaspar4u wants to merge 2 commits into
NousResearch:mainfrom
Diaspar4u:fix/platform-session-isolation-overrides
Open

Diaspar4u wants to merge 2 commits into
NousResearch:mainfrom
Diaspar4u:fix/platform-session-isolation-overrides

Conversation

@Diaspar4u

@Diaspar4u Diaspar4u commented Aug 13, 2026 •

Copy link
Copy Markdown

What does this PR do?

Makes every current gateway consumer honor the same effective per-platform group_sessions_per_user and thread_sessions_per_user policy.

Per-platform overrides now survive config loading and flow through session keying, bare-runner fallback keying, shared-session sender attribution, prompt context, live and persisted /resume authorization, Discord prospective-thread persistence, and durable profile-safe recovery. Routing still falls back to the global gateway defaults when no platform override exists.

Lineage and related work

Supersedes #81207 while preserving the original isolation contribution from #81208 with public co-author attribution. #84926 remains stacked on this contract for its separate authorized WhatsApp group-observation behavior and will be restacked after this base PR.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes session routing and authorization consistency)

Changes Made

  • Preserve a top-level platform block's nested extra map, including extra-only blocks, while retaining top-level bridged-key precedence.
  • Add one config resolver for effective group/thread isolation.
  • Route SessionStore, runner fallback, sender attribution, context construction, and resume authorization through that policy.
  • Keep Discord prospective-thread routing Discord-only and preserve the existing rule that DMs ignore prospective thread metadata.
  • Persist the same effective Discord thread/chat identity used by the session key.
  • Prefer durable profile_name ownership during recovery; use the requested namespace in multiplex mode and the active owner in single-profile mode.
  • Port behavior into current-main owners: config_loader.py, run_inbound.py, session_recovery.py, and slash_commands_session.py.
  • Preserve the original contributor through the commit's public co-author trailer.

Review feedback disposition

All prior findings and suggestions remain explicitly resolved:

  1. build_session_context() calls the same per-platform resolver used by keying and authorization; no inline isolation lookup remains.
  2. Prospective-thread promotion is restricted to Discord. Stray prospective metadata on Telegram or another platform preserves its existing group identity, and Discord DMs preserve their real-thread-only behavior.
  3. feat(whatsapp): observe authorized group context before response #84926 is treated as a dependent patch and will be restacked rather than duplicating this base.
  4. feat(gateway): honor adapter-declared session scope for plugin platforms #99950 owns plugin/out-of-tree adapter registration. This PR does not duplicate that registry, but every runner consumer now consults the real store first so the two policies compose.
  5. Durable profile recovery remains here because changing isolated keys to shared keys can recover an older row through peer fallback. Durable ownership preserves continuity for the correct profile and rejects another profile's row.

No maintainer review or inline finding is pending.

How to Test

./scripts/run_tests.sh tests/gateway/test_config.py tests/gateway/test_session.py tests/gateway/test_resume_command.py tests/gateway/test_run_progress_topics.py tests/gateway/test_session_identity_restore.py

The five focused config/session invariants fail against current upstream main with dropped nested config, divergent per-user keys, mismatched prospective-thread persistence, and profile-unsafe recovery; they pass on this head.

Verification

  • 224 focused config/session/resume/Discord-topic tests passed.
  • Focused Ruff passed.
  • Plugin-compat pointer audit passed.
  • Contributor attribution audit passed for Andrey and the preserved co-author.
  • git diff --check passed.

Checklist

Code

  • I've read the current contributing guide and area instructions.
  • My commit message follows Conventional Commits.
  • I inspected the complete PR thread, review suggestions, and related/stacked PRs.
  • Every changed consumer resolves the same effective isolation policy.
  • The focused invariants fail without the patch and pass with it.
  • Tested on macOS 26.6.1 with Python 3.11.

Documentation & Housekeeping

  • User/config documentation: N/A — existing keys and defaults are unchanged.
  • Architecture follows current decomposed gateway owners.
  • Cross-platform behavior considered — routing and SQLite metadata are platform-independent; Discord prospective-thread behavior is explicitly bounded.
  • Tool schema documentation: N/A — no tool schema changed.

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery area/config Config system, migrations, profiles P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 13, 2026
@Diaspar4u
Diaspar4u force-pushed the fix/platform-session-isolation-overrides branch from 0c4a482 to 6af89e8 Compare August 13, 2026 12:58
@Diaspar4u

Copy link
Copy Markdown
Author

Re-review requested. Closed the remaining per-platform isolation gaps in authorization, persisted /resume scoping, Discord prospective-thread attribution, and config loading while keeping the PR focused on session isolation overrides.

Validation: 152 focused gateway tests passed; focused Ruff, diff checks, and SSH signature verification passed.

@Diaspar4u
Diaspar4u force-pushed the fix/platform-session-isolation-overrides branch from 6af89e8 to d8b6490 Compare August 13, 2026 17:00
@Diaspar4u

Diaspar4u commented Aug 13, 2026 •

Copy link
Copy Markdown
Author

Rebased the completed isolation fix onto current main at head d8b6490fd6; the only overlapping upstream change in gateway/run.py was preserved. All four upstream-range commits have verified SSH signatures.

Validation: 164 focused gateway tests passed; focused Ruff, git diff --check, and upstream-range diff checks passed.

GitHub Actions for exact head d8b6490fd67e22af9265aaabad5274c24dffc388 are awaiting write-access approval; please approve the CI and Docker workflow runs.

@coderabbitai review

Maintainers: please re-review the current head.

@Diaspar4u

Copy link
Copy Markdown
Author

@teknium1 @alt-glitch please re-review the current head.

@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference, author can ignore or act on any point.

fix(gateway): honor per-platform session isolation overrides

Centralizing resolution in resolve_session_isolation() and threading it through the key/attribution/auth paths is the right approach, and the override tests are good. Observations:

  1. Duplicated resolution in build_session_context. gateway/session.py build_session_context re-implements the platform-extra lookup inline (platform_extra.get(...)) instead of calling resolve_session_isolation(config, source) — two implementations that can drift. Consider having it call the helper.
  2. is_shared_multi_user_session semantic change via effective_session_thread_id. Switching from source.thread_id to thread_id or prospective_thread_id changes shared/isolated classification for all platforms with prospective threads, not just the ones under test — a prospective thread now keys off thread_sessions_per_user instead of group_sessions_per_user. The tests document the intent, but confirm this flip is wanted platform-wide, since it also drives /resume scoping and the session-context prompt, not just keying.
  3. Overlap with feat(whatsapp): observe authorized group context before response #84926. The session-isolation hunks here (config extra preservation, resolve_session_isolation, effective_session_*) also appear in feat(whatsapp): observe authorized group context before response #84926 — whichever merges second will conflict/duplicate. Coordinate so only one copy lands.

@Diaspar4u

Copy link
Copy Markdown
Author

Addressed all three points on current head c141281d68da:

  1. Fixed the duplicated resolution path. build_session_context() now calls the same resolve_session_isolation() helper used by keying, authorization, and attribution.
  2. Bound the prospective-thread behavior to its actual contract. Only Discord can promote prospective_thread_id into thread routing/classification; a new regression proves stray prospective metadata on another platform preserves its existing group key and per-user isolation. The existing Discord initiate-in-channel/continue-in-thread behavior and sender attribution remain covered and passing.
  3. Confirmed the stack coordination. feat(whatsapp): observe authorized group context before response #84926 is explicitly documented as stacked on this PR and will be refreshed onto main after fix(gateway): honor per-platform session isolation overrides #84925 merges, so the shared isolation commits land only once.

Validation: 165 existing and new session/resume/config/WhatsApp isolation tests passed; focused Ruff, attribution audit, both diff checks, and all six upstream-range SSH signature checks passed.

@teknium1 @alt-glitch please re-review the materially changed current head.

@Diaspar4u

Diaspar4u commented Aug 17, 2026 •

Copy link
Copy Markdown
Author

Fixed profile-safe recovery when a per-user session key becomes shared: durable session ownership now wins over the legacy key namespace, while rows owned by another profile remain rejected. Added continuity and cross-profile rejection regressions.

Current head e9dde73b6f883100dff930538e5c5e01d26f7c8a preserves the validated patch exactly. Validation: 165 focused tests passed; focused Ruff, diff checks, attribution audit, and all upstream-range commit signatures passed.

@teknium1 @alt-glitch please re-review the current head. Actions on this exact SHA are awaiting write-access approval; please approve the workflow runs.

@Diaspar4u
Diaspar4u force-pushed the fix/platform-session-isolation-overrides branch from 9e363b6 to e9dde73 Compare August 18, 2026 07:15
@Diaspar4u

Copy link
Copy Markdown
Author

Final disposition on #81207: #84925 is the preferred replacement. It carries #81207’s two session-isolation commits with original authorship preserved, then applies the same effective per-platform policy consistently to authorization, durable /resume scoping, Discord prospective-thread attribution, shared-session sender attribution, canonical context construction, and nested platform configuration loading. Those additional boundaries prevent session keying from disagreeing with access, replay, or attribution behavior. No unique isolation-fix scope remains in #81207. #81208 remains separate only for its WhatsApp mention-tagging behavior.

@Diaspar4u

Copy link
Copy Markdown
Author

Significant-overlap disposition: #62370 fixes one remaining global group/thread-precedence case inside is_shared_multi_user_session; #84925 instead resolves effective per-platform overrides and applies that policy consistently across keying, authorization, /resume, context construction, and Discord attribution. #62370 is a complementary narrow helper correction, not a replacement for #84925’s broader consistency contract. #47794 is also complementary: it preserves each appended author inside WhatsApp’s debounce batch, while #84925 governs isolation and attribution after the event reaches gateway session handling. Consolidation must retain those two narrow fixes, but neither makes #84925 unnecessary.

@Diaspar4u

Copy link
Copy Markdown
Author

Further relationship disposition: #13939 deliberately decouples sender attribution from session isolation by adding a default-on stable [from … uid:…] prefix in every chat context. #84925 keeps the existing attribution model but makes isolation, authorization, resume visibility, and shared-session attribution agree on the same effective per-platform policy. #13939 is a broader competing attribution contract, not a replacement for #84925’s isolation fix. #75770 is a behavior-preserving extraction of resume authorization from slash_commands.py; if it lands first, #84925’s changed authorization logic must be ported into the new mixin. That structural refactor does not remove #84925’s semantic requirement.

@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference — not a maintainer.

Thanks for the disposition — noted that #13939 is a broader competing attribution contract rather than a replacement for #84925's isolation fix, and that #75770 only requires porting the authorization logic if it lands first.

@Diaspar4u
Diaspar4u force-pushed the fix/platform-session-isolation-overrides branch from e9dde73 to fec7e34 Compare August 20, 2026 16:13
@Diaspar4u

Diaspar4u commented Aug 20, 2026 •

Copy link
Copy Markdown
Author

Resolved the contributor-identity exposure at its source. The carried contributor commits now use the verified public GitHub noreply identity, the mapping-only commit and both added mapping files are gone, and the code/test patch is unchanged by range-diff. The stale dirty-worktree blocker came from two upstream mapping filenames that differed only by case; current main deletes both, and this branch now includes that upstream fix. Current head fec7e34dbd10fdc04c36a1307698ebf27be1d343 is mergeable. Validation: 160 focused tests passed; focused Ruff and diff checks passed. @teknium1 @alt-glitch please review the sanitized current head. Actions on this exact SHA are awaiting write-access approval; please approve those workflow runs.

@Diaspar4u
Diaspar4u force-pushed the fix/platform-session-isolation-overrides branch from fec7e34 to 9cdb7a7 Compare August 22, 2026 18:56
@Diaspar4u

Diaspar4u commented Aug 22, 2026 •

Copy link
Copy Markdown
Author

Updated at 9cdb7a76e865bcd677ed6ba20a1ad2fa231fc913: preserved the two carried contributor commits and normalized the four Andrey-authored commits to the canonical public GitHub identity; the code and test patch is unchanged. Validation: 172 focused gateway tests passed, focused Ruff passed, and diff plus current-main merge checks passed. @teknium1 @alt-glitch please review this exact head and approve its pending Actions workflows.

@srosro

srosro commented Sep 1, 2026 •

Copy link
Copy Markdown

+1 on this shape over #81207: routing every consumer (store, run.py fallback key, sender attribution, session context, _same_origin_chat, _resume_target_allowed) through one resolve_session_isolation() is what keeps the auth guards in lock-step with the keys — fixing derivation alone leaves a shared-key group with per-user resume/auth behavior. The only-extra platform block fix (the continue guard) also matters; #81207 skips that case.

Two suggestions from having hit this bug class in production (an iMessage-plugin deployment where extra.group_sessions_per_user: false never reached the session store, so a family group chat cross-contaminated per-user contexts — one member's reply was answered with another member's unrelated task thread):

  1. Plugin/relay platforms fall through this fix. resolve_session_isolation reads config.platforms[source.platform].extra, so an out-of-tree platform adapter (plugin-provided, or relay-fronted where the logical platform has no entry in the gateway's platforms map) still silently falls back to the globals — the exact failure we hit. A cheap extension: let the resolver consult the owning adapter's declared scope (or a small platform→scope registry adapters populate at init) before the global fallback. Reference implementation of that mechanism: fix(gateway): single owner for session-scope resolution (group chats split into per-sender sessions) srosro/hermes-agent#4 (happy for any of it to be lifted).

  2. The durable-profile-ownership change in _recovered_row_allowed_for_active_profile is a separable concern — splitting it out would make this easier to review/merge quickly.

Happy to test a build of this against our deployment.

EDIT: Opened the complementary PR for the plugin/out-of-tree gap described above: #99950 — adapters register their declared scope with the session store at acceptance; the registry consult sits above the config fallback, so it composes with this PR whichever merges first.

@Diaspar4u

Copy link
Copy Markdown
Author

Final disposition on both suggestions:

  1. Confirmed: the plugin/relay gap is complementary to fix(gateway): honor per-platform session isolation overrides #84925 rather than part of its config-map contract. feat(gateway): honor adapter-declared session scope for plugin platforms #99950 now owns the adapter-declared registry path, so this PR will not duplicate it. The eventual composition must resolve accepted adapter registration → per-platform config → global defaults while preserving every key, authorization, attribution, and context consumer.
  2. The durable-profile recovery stays in this PR. Enabling a known platform’s shared override changes the key shape and can recover the prior isolated row through peer fallback; durable profile_name ownership preserves that continuity while rejecting a row owned by another profile. Splitting it would leave the isolation transition incomplete.

No patch change is required on #84925.

@Diaspar4u
Diaspar4u force-pushed the fix/platform-session-isolation-overrides branch from e736dae to 018c8e4 Compare September 3, 2026 07:11
@Diaspar4u

Copy link
Copy Markdown
Author

Resolved the current-main conflict in durable profile recovery: single-profile rows remain fenced by the active durable owner, while multiplexed rows now use the requested profile namespace rather than the process-active profile. Added matching/mismatching durable-owner regression coverage.

Validation on head 018c8e4c425dbd5f7d664bd9e358a29793ee646b: 188 focused gateway tests passed; focused Ruff, git diff --check, attribution, and current-main merge checks passed.

@teknium1 @alt-glitch please review this exact head. A write-access maintainer: please approve its pending Actions workflows.

@Diaspar4u
Diaspar4u force-pushed the fix/platform-session-isolation-overrides branch from 018c8e4 to b7bff37 Compare September 6, 2026 01:09
@Diaspar4u

Copy link
Copy Markdown
Author

@teknium1 Rebased this PR onto current main and ported the isolation contract into the current config-loader, inbound, recovery, and session-command owners. Current head: b7bff37fb4956900b5e2bfb325bf2e5ce9849989.

Every current consumer now resolves the same per-platform policy: store and bare-runner keys, sender attribution, prompt context, live/persisted resume authorization, Discord prospective-thread persistence, and durable recovery. The port also preserves current-main's DM rule by ignoring prospective thread metadata in Discord DMs.

All prior feedback remains resolved. #84926 stays dependent and will be restacked instead of duplicating this base. #99950 remains complementary for adapter-declared/plugin scope; runner consumers now consult the real store before the per-platform-config fallback, preserving the intended composition. Durable profile ownership remains here because it is required to recover an isolated row into a shared key without crossing profile boundaries. #62370, #47794, #13939, and #75770 remain separately owned as documented in the body.

Validation: 192 focused config/session/resume/Discord-topic tests passed; focused Ruff, compatibility-pointer, attribution, and diff checks pass. Five focused invariants fail on current main and pass here. The original contributor remains credited through the public co-author trailer.

This head is mergeable. CI, Nix, and Docker are action_required; please review and approve Actions for this exact head.

@Diaspar4u
Diaspar4u force-pushed the fix/platform-session-isolation-overrides branch from b7bff37 to 3219c9f Compare September 8, 2026 07:24
@Diaspar4u

Copy link
Copy Markdown
Author

Rebased onto current upstream main at head 3219c9f. The shared isolation resolver still governs session keys, sender attribution, context, recovery, and resume authorization; the current platform-event extraction is integrated without widening the patch.

Validation: 185 focused tests passed; focused Ruff, Windows-footgun, plugin-compat, attribution, and diff checks passed. The preserved co-author remains attributed.

@teknium1 @alt-glitch please review this exact head. A write-access maintainer: please approve its Actions workflows.

srosro added a commit to srosro/hermes-agent that referenced this pull request Sep 8, 2026
SessionStore derived group_sessions_per_user / thread_sessions_per_user
from the global gateway config only, so a plugin / out-of-tree platform
adapter that declares its own isolation in config.extra was silently
ignored — it has no entry in config.platforms for the per-platform fixes
(NousResearch#81207 / NousResearch#84925) to read. Production incident: an iMessage-plugin family
group chat with group_sessions_per_user=false split into per-user
sessions and the agent answered one member with another's task thread.

Add the missing seam: adapters register their resolved scope with the
store when their handlers are wired (before connect(), where Telegram
polling / webhooks go inbound-reachable), and every key derivation and
every guard that must agree with key shape resolves through it before
the config defaults.

- SessionStore.register_platform_session_scope / resolve_session_scope,
  keyed (profile, platform) so multiplexed profiles can't leak overrides;
  _generate_session_key resolves through it. Registry lives behind the
  store's _lazy() helper like its other optional maps.
- GatewayAdapterLifecycleMixin._register_adapter_session_scope runs from
  _wire_adapter_handlers (primary + secondary via _configure_profile_
  adapter). A missing / explicit-None flag is seeded from the OWNING
  gateway config (a secondary profile's, not the primary's) and written
  back to config.extra so adapter-side key derivation agrees; the
  instantiate-time primary-config seed is deleted.
- GatewayRunner._resolve_session_scope_for is the one runner-side read of
  isolation flags: _session_key_for_source's no-store fallback, the
  inbound sender-attribution gate, and the /resume IDOR guards use it;
  build_session_context takes session_store so the agent-facing
  shared_multi_user_session flag agrees with the key.
- One key owner everywhere: /undo evicts under _session_key_for_source,
  the voice-handoff destination uses it too (_handoff_session_key
  deleted; dest.source already carries the queuing profile), and Slack's
  thread / rehydration keys resolve through the store under the
  adapter's owning profile.
- config_session_scope(config) owns the config-default pair.

Tests: registered override beats the global default, profile keying
(multiplexed), context flag follows scope, owning-profile seeding, the
IDOR guard flipping with the registered scope + runner key byte-identical
to the store key (real store), and a secondary Slack bot's thread keys
following its own profile's registration.

Composes with NousResearch#84925 (registry -> platform-config -> defaults). Adjacent:
NousResearch#81860's handoff destination-shape ownership is unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lqr7yMugdYt53YxrszjnSG
@Diaspar4u
Diaspar4u force-pushed the fix/platform-session-isolation-overrides branch 4 times, most recently from 9c50a47 to 24325b1 Compare September 16, 2026 07:17
@Diaspar4u
Diaspar4u force-pushed the fix/platform-session-isolation-overrides branch 2 times, most recently from 7760e0a to c10a6df Compare September 19, 2026 07:19
@Diaspar4u

Copy link
Copy Markdown
Author

Current-main compatibility is resolved on head c10a6df305513afd19b616114cbedceb57f85933: the session-isolation patch is unchanged, and the overlapping stalled-session spool test now uses explicit UTF-8 so the current Windows-footgun gate passes. Validation on this exact head: 219 focused config/session/resume/topic tests passed; focused Ruff, Windows-footgun, plugin-compat, attribution, and diff checks passed. @teknium1 @alt-glitch, please review this head; a write-access maintainer, please approve its CI, Nix, and Docker Actions for this exact SHA.

@Diaspar4u
Diaspar4u force-pushed the fix/platform-session-isolation-overrides branch from c10a6df to ed7922f Compare September 21, 2026 15:25
Diaspar4u and others added 2 commits September 22, 2026 03:13
Co-authored-by: dhruv kejriwal <96516827+dhruvkej9@users.noreply.github.com>
@Diaspar4u
Diaspar4u force-pushed the fix/platform-session-isolation-overrides branch from ed7922f to c3f3351 Compare September 22, 2026 07:16

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants